Make some const prop mir-opt tests unit-tests
#99770
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @JakobDegen |
This has been discussed a number of times, and people have mixed opinions on it. I still think we should do it though. I think there was a Zulip thread about this somewhere. I'll review later as well, but no perms so Mark will need to review himself or hand off to mir-opt or something |
|
I'm going to wait to review this until after #99780 lands, for obvious reasons |
…-obk Use line numbers relative to the function in mir-opt tests As shown in rust-lang#99770, the line numbers can be a big source of needless and confusing diffs. This PR adds a new flag `-Zmir-pretty-relative-line-numbers` to make them relative to the function declaration, which avoids most needless diffs from attribute changes. `@JakobDegen` told me that there has been a zulip conversation about disabling line numbers with mixed opinions, so I'd like to get some feedback here, for this hopefully better solution. r? rust-lang/wg-mir-opt
|
☔ The latest upstream changes (presumably #99780) made this pull request unmergeable. Please resolve the merge conflicts. |
Noratrieb
force-pushed
the
mir-pass-unit-test
branch
from
e39a0d9 to
ea33432
Compare
There was a problem hiding this comment.
Overall looks good. Unfortunately, const prop currently uses the mir-opt-level to determine how much constant propagation to do, which plays poorly with // unit-test since that emits -Z mir-opt-level=0. This causes a couple of regressions (see below) in places where const prop is now less aggressive because of the flag. Imo it's a bad idea to have the same flag control both whether other optimizations run and how aggressive const prop is. I think we should instead have -Z mir-const-prop-level which defaults to the same value as -Z mir-opt-level but can be set separately. Assuming Oli agrees with this assessment, after this is merged can you file a follow-up issue to that effect?
Edit: I hit this in an unrelated case, so I've gone and opened a Zulip thread about it
There was a problem hiding this comment.
This is one example but probably non-critical
src/test/mir-opt/const_prop/invalid_constant.main.ConstProp.diff
Outdated
Show resolved
Hide resolved
Noratrieb
force-pushed
the
mir-pass-unit-test
branch
from
ea33432 to
ee30cc8
Compare
|
I forgot about this PR :D, but fixed the one bad case now. |
|
Let's reroll to get it through |
|
r? rust-lang/mir-opt |
|
it does not want to do the thing :/ |
|
Lets just r? @wesleywiser since he reviewed a similar PR a bit ago |
|
@bors r+ |
bors
added
S-waiting-on-bors
…-obk Make some const prop mir-opt tests `unit-test`s Most of these have no or only tiny diffs beyond line numbers being changed (would it make sense to not have line numbers in mir-opt tests?). Some things changed a bit, but I think it should all be fine, not sure though.
Rollup of 8 pull requests Successful merges: - rust-lang#98200 (Expand potential inner `Or` pattern for THIR) - rust-lang#99770 (Make some const prop mir-opt tests `unit-test`s) - rust-lang#99957 (Rework Ipv6Addr::is_global to check for global reachability rather than global scope - rebase) - rust-lang#100331 (Guarantee `try_reserve` preserves the contents on error) - rust-lang#100336 (Fix two const_trait_impl issues) - rust-lang#100713 (Convert diagnostics in parser/expr to SessionDiagnostic) - rust-lang#100820 (Use pointer `is_aligned*` methods) - rust-lang#100872 (Add guarantee that Vec::default() does not alloc) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
7 participants
Most of these have no or only tiny diffs beyond line numbers being changed (would it make sense to not have line numbers in mir-opt tests?). Some things changed a bit, but I think it should all be fine, not sure though.